-
-
Notifications
You must be signed in to change notification settings - Fork 272
[LLVM 21] fix calls to llvm.lifetime.start
#4979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
aacda96 to
812d365
Compare
|
Did you check whether those lifetime calls are there for LLVM 20? (in the exact same functions?) Would help to get a standalone smaller testcase btw. Something like this: https://d.godbolt.org/z/d6f9763KE |
|
I think I found the extremely weird answer in the clang source (see #4990) - the calls still feature the size argument, although the declaration has a single pointer parameter: https://github.com/llvm/llvm-project/blob/220bac16a417e97bf97fdcb34855e28b2e6dfdf7/clang/lib/CodeGen/CGDecl.cpp#L1363 |
|
that is weird! |
812d365 to
9ebd597
Compare
|
reduced to struct CodepointSet
{
CodepointSet opBinary(string op, U)(U )
{
return this;
}
this(this) { }
}
CodepointSet memoizeExpr(string expr)()
{
if (__ctfe)
return mixin(expr);
CodepointSet slot;
return slot;
}
void wordCharacter() {
memoizeExpr!"unicode.A | unicode.M";
}
struct unicode
{
static CodepointSet opDispatch(string name)()
{
CodepointSet set;
return set;
}
}it generates a broken call ; Function Attrs: uwtable(sync)
define weak_odr void @_D7package__T11memoizeExprVAyaa21_756e69636f64652e41207c20756e69636f64652e4dZQCmFNaNbNiNfZSQDl__T13InversionListTSQEi8GcPolicyZQBe(ptr noalias sret(%"package.InversionList!(GcPolicy).InversionList") align 1 %.sret_arg) #0 {
%slot = alloca %"package.InversionList!(GcPolicy).InversionList", align 1
%__copytmp3 = alloca %"package.InversionList!(GcPolicy).InversionList", align 1
call void @llvm.lifetime.start.p0(ptr captures(none) %slot) #2
call void @llvm.memset.p0.i64(ptr align 1 %slot, i8 0, i64 1, i1 false)
call void @llvm.lifetime.start.p0(ptr captures(none) %.sret_arg) #2 ; <<<<<< HERE
call void @llvm.memcpy.p0.p0.i64(ptr align 1 %.sret_arg, ptr align 1 %slot, i64 1, i1 false)
call void @_D7package__T13InversionListTSQBc8GcPolicyZQBe15__fieldPostblitMFNaNbNiNeZv(ptr nonnull %.sret_arg) #0
ret void
} |
|
53d7c1d is not the correct fix, and fails when built with recent LLVM. and removing the early return from if (lltype->isVoidTy()) {
irLocal->value = getNullPtr();
return;
}does not fix it either. I think this is a DMD bug where if (__ctfe)
return mixin(expr);
alias T = typeof(mixin(expr));
T slot;
return slot; // This NRVO is broken
}the |
9ebd597 to
69b3f0e
Compare
|
Hopefully this is now fixed. |
5424e87 to
0e742c1
Compare
It's been some time, but CI back then clearly showed that the 2 args are still required for LLVM 21. They might have switched to the single arg in LLVM 22+.
My commit doesn't just remove that wrong early return you've introduced, but has another logic change ( |
|
I realised what you were fixing in that commit, which was the |
| allocainst = getNullPtr(); | ||
| } else if (type != vd->type) { | ||
| irLocal->value = getNullPtr(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The missing else here compared to my commit now means that this line above has no more effect for IR-void-typed vars; irLocal->value is immediately overwritten below with a dummy void-alloca (probably promoted to some dummy i8 storage). And now gets DI infos.
Edit: And additionally caused you to come up with a weird logic for the lifetime-start annotation apparently. So please just try my commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted wr.t. DI infos, fixed that. Nope:
LLType *lltype = DtoType(type); // void for noreturn
if (lltype->isVoidTy()) {
irLocal->value = getNullPtr();
} else {
auto allocainst = type != vd->type ? DtoAlloca(type, vd->toChars())
: DtoAlloca(vd, vd->toChars());
irLocal->value = allocainst;
gIR->DBuilder.EmitLocalVariable(allocainst, vd);
// The lifetime of a stack variable starts from the point it is declared
gIR->funcGen().localVariableLifetimeAnnotator.addLocalVariable(
allocainst, DtoConstUlong(size(type)));
}still fails with
llvm.lifetime.start can only be used on alloca or poison
call void @llvm.lifetime.start.p0(ptr captures(none) %0) #4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding back in the if
if (!vd->isParameter() && !varIsSret(vd, gIR->func())) {
gIR->funcGen().localVariableLifetimeAnnotator.addLocalVariable(
allocainst, DtoConstUlong(size(type)));
}then causes it to pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternately if (AFAIU) the entire point of these lifetime annotations is to aid debugging, then we could restrict them to only user variables (i.e. ig more all compiler generated ones beginning with __, e.g. __copytemp which in addition to __tmpfordtor and __sl seems to cover all the bases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well IIRC, Phobos couldn't be compiled successfully in #4990 after cherry-picking an earlier version of this PR's commit, but applying both linked commits I linked made it work with LLVM 21 and older versions at least. I think I understood what you were after with the earlier unrelated refactoring (combining the DI and lifetime-start conditions to non-IR-void-typed vars only, without the realAlloca helper var and a 2nd void check), that made sense; it's just that the early return instead of an else branch caused skipping some stuff at the end of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note that the sret NRVO case is handled just above in an earlier if:
Lines 887 to 900 in b5f7bb0
| } else if (gIR->func()->sretArg && | |
| ((gIR->func()->decl->isNRVO() && | |
| gIR->func()->decl->nrvo_var == vd) || | |
| (vd->isResult() && !isSpecialRefVar(vd)))) { | |
| // Named Return Value Optimization (NRVO): | |
| // T f() { | |
| // T ret; // &ret == hidden pointer | |
| // ret = ... | |
| // return ret; // NRVO. | |
| // } | |
| assert(!isSpecialRefVar(vd) && "Can this happen?"); | |
| getIrLocal(vd, true)->value = gIR->func()->sretArg; | |
| gIR->DBuilder.EmitLocalVariable(gIR->func()->sretArg, vd); | |
| } else { |
That's where the lval is set to the sret argument instead of an alloca. And where no lifetime-start annotation is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but as your interesting reduced test case shows, that's not enough. In-place construction can replace a temporary alloca later with an sret argument, as the lvalue of a variable:
Lines 3002 to 3015 in b5f7bb0
| // and temporaries | |
| else if (isTemporaryVar(rhs)) { | |
| Logger::println("success, in-place-constructing temporary"); | |
| auto lhsLVal = DtoLVal(lhs); | |
| auto rhsLVal = DtoLVal(rhs); | |
| if (!llvm::isa<llvm::AllocaInst>(rhsLVal)) { | |
| error(rhs->loc, "lvalue of temporary is not an alloca, please " | |
| "file an LDC issue"); | |
| fatal(); | |
| } | |
| if (lhsLVal != rhsLVal) | |
| rhsLVal->replaceAllUsesWith(lhsLVal); | |
| return true; | |
| } |
According to the -vv log, this is what happens in the last memoizeExpr return statement:
ExpStatement::toIR(): <source>(15)
* D to dwarf stoppoint at line 15, column 5
* DeclarationExp::toElem: (CodepointSet slot = 0;) | T=void
* * DtoDeclarationExp: slot
* * * VarDeclaration
* * * DtoVarDeclaration(vdtype = CodepointSet)
* * * * llvm value for decl: %2 = alloca %example.CodepointSet, align 1
* * * * expression initializer
* * * * AssignExp::toElem: slot = 0 | (CodepointSet)(CodepointSet = int)
* * * * * VarExp::toElem: slot @ CodepointSet
* * * * * * DtoSymbolAddress ('slot' of type 'CodepointSet')
* * * * * * * a normal variable
* * * * * attempting in-place construction
* * * * * * aborted due to different base types without modifiers
* * * * * IntegerExp::toElem: 0 @ int
* * * * * * IntegerExp::toConstElem: 0 @ int
* * * * * * * value = i32 0
* * * * * performing aggregate zero initialization
ReturnStatement::toIR(): <source>(16)
* D to dwarf stoppoint at line 16, column 5
* attempting in-place construction
* * success, in-place-constructing temporary
* * CommaExp::toElem: (CodepointSet __copytmp3 = (__copytmp3 = slot).this(this)();) , __copytmp3 @ CodepointSet
* * * DeclarationExp::toElem: (CodepointSet __copytmp3 = (__copytmp3 = slot).this(this)();) | T=void
* * * * DtoDeclarationExp: __copytmp3
* * * * * VarDeclaration
* * * * * DtoVarDeclaration(vdtype = CodepointSet)
* * * * * * llvm value for decl: %3 = alloca %example.CodepointSet, align 1
* * * * * * expression initializer
* * * * * * CallExp::toElem: (__copytmp3 = slot).this(this)() @ void
* * * * * * * DotVarExp::toElem: (__copytmp3 = slot).this(this) @ void()
* * * * * * * * AssignExp::toElem: __copytmp3 = slot | (CodepointSet)(CodepointSet = CodepointSet)
* * * * * * * * * VarExp::toElem: __copytmp3 @ CodepointSet
* * * * * * * * * * DtoSymbolAddress ('__copytmp3' of type 'CodepointSet')
* * * * * * * * * * * a normal variable
* * * * * * * * * attempting in-place construction
* * * * * * * * * VarExp::toElem: slot @ CodepointSet
* * * * * * * * * * DtoSymbolAddress ('slot' of type 'CodepointSet')
* * * * * * * * * * * a normal variable
* * * * * * * * * performing normal assignment (rhs has lvalue elems = 1)
* * * * * * * * * DtoAssign()
* * * * * * * DtoCallFunction()
* * * * * * * * Building type: void()
* * * * * * * * * DtoFunctionType(void())
* * * * * * * * * * x86-64 ABI: Transforming argument types
* * * * * * * * * * Final function type: void ()
* * * * * * * * doing normal arguments
* * * * * * * * Arguments so far: (1)
* * * * * * * * * %3 = alloca %example.CodepointSet, align 1
* * * * * * * * Function type: void()
* * * VarExp::toElem: __copytmp3 @ CodepointSet
* * * * DtoSymbolAddress ('__copytmp3' of type 'CodepointSet')
* * * * * a normal variable
So __copytmp3 first gets an alloca, and then that lval is IR-replaced with the sret argument later, incl. for the lifetime-start call. Your diagnosis might be right wrt. __copytmp3 apparently not being the frontend-NRVO variable directly; the return expression in the if (__ctfe) block might break it (as the NRVO var must normally be returned in every return statement of the function).
Edit: But there are always going to be remaining non-NRVO cases. So for these lifetime annotations, we might have to skip this in-place construction of sret values from non-NRVO temporaries (edit: oh well, the extra memcpy and different address might break code depending on RVO though...). Or scan the existing uses, to remove the lifetime intrinsic calls (possibly missing the lifetime end calls if they are pushed later on...), before replacing all uses.
Edit2: I think what would probably be best is to pre-set the lval (i.e., getIrLocal(vd, true)->value) of the temporary to the sret argument in toInPlaceConstruction(), before evaluating the comma-expression. [Incl. emitting DI there, as DtoVarDeclaration() wouldn't do it anymore due to isIrLocalCreated(vd).] Instead of replacing all IR uses after a normal evaluation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your diagnosis might be right wrt. __copytmp3 apparently not being the frontend-NRVO variable directly; the return expression in the if (__ctfe) block might break it (as the NRVO var must normally be returned in every return statement of the function).
Yeah, an if(false) return ...; also breaks NRVO. I opened a DMD issue for this.
The removal of isRealAlloca was mostly to try to use the type system to fix/make sure I wasn't doing anything stupid with it, but it seems that the replaceAllUsesWith is what is causing it to change type from an AllocaInst to Argument. Thank you for finding that, I thought I was going mad!
It seems to me that the easiest solution then would be to strip the calls to @llvm.lifetime prior to the replaceAllUsesWith, though I don't quite understand what you are trying to say in your Edit2.
Anyway I've added two of the reductions to the test suite (there is another one with __tmpfordtor that I ned to re-run). I'm going away for the weekend, so if you feel like implementing any of your suggestions, please do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good find!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so if you feel like implementing any of your suggestions, please do.
Okay, I've tried: #5015. I'm pretty sure I've added that replace-all-uses-with hack, so only fair to clean it up now. :)
0e742c1 to
06344a3
Compare
Its main motivation is for performance (reuse of stack memory, which also helps against stack exhaustion) |
This mostly fixes compilation for LLVM21, except for some breakage with regards to moving
llvm.lifetime.startto taking only andAllocaInst(or aPoison).This both produces a call to
@llvm.lifetime.start.p0(ptr captures(none) %0)and%0in this case is anArgumentnot anAllocaInst.I'm trying to figure out where those calls are coming from. Is it the ABI messing things up somehow? I don't know what is special about those functions.
there is only the one use of it and